Hey Aditya, sharp catch on the modulo wrap-around bug — when a vehicle reaches its last stop, silently returning the first stop as the "next stop" is exactly the kind of subtle data corruption that's easy to miss and hard to debug downstream. The fix is clean and correct. A couple of items before merging:

Important

1. Add a unit test for findNextStop (internal/restapi/trips_helper.go:291)

This function currently has zero test coverage (no tests reference findNextStop anywhere in the codebase). Since this is a bugfix, a regression test would ensure this behavior doesn't silently revert. Three cases would cover it well:

func TestFindNextStop(t *testing.T) {
    api := createTestApi(t)

    stopTimes := []*gtfsdb.StopTime{
        {StopID: "A", StopSequence: 1},
        {StopID: "B", StopSequence: 2},
        {StopID: "C", StopSequence: 3},
    }

    t.Run("returns next stop when not at last stop", func(t *testing.T) {
        seq := uint32(1)
        vehicle := &gtfs.Vehicle{CurrentStopSequence: &seq}
        stopID, _ := findNextStop(api, stopTimes, vehicle)
        assert.Equal(t, "B", stopID)
    })

    t.Run("returns empty when at last stop", func(t *testing.T) {
        seq := uint32(3)
        vehicle := &gtfs.Vehicle{CurrentStopSequence: &seq}
        stopID, _ := findNextStop(api, stopTimes, vehicle)
        assert.Equal(t, "", stopID)
    })

    t.Run("returns empty when vehicle is nil", func(t *testing.T) {
        stopID, _ := findNextStop(api, stopTimes, nil)
        assert.Equal(t, "", stopID)
    })
}

Fit and Finish

2. api parameter is unused (trips_helper.go:292)

The api *RestAPI parameter isn't referenced anywhere in findNextStop. This is a pre-existing issue (not introduced by your PR), so fixing it is optional — but since you're already touching this function, removing it would clean things up. If you do, update the call site at line 102:

// Line 102, before:
nextStopID, nextOffset = findNextStop(api, stopTimesPtrs, vehicle)

// After:
nextStopID, nextOffset = findNextStop(stopTimesPtrs, vehicle)

Verification

• make test: all packages pass
• make lint: 0 issues
• gofmt -d: no formatting differences
• Single commit: d7a3305 fix: prevent findNextStop from wrapping to first stop at end of trip

Strengths

• Correct diagnosis: The PR description clearly explains the bug, the root cause (modulo wrap-around), and the downstream effects (incorrect numberOfStopsAway, distanceFromStop, arrival predictions). Well-articulated.
• Minimal, focused fix: One function, 4 lines changed, no unnecessary refactoring.
• Dead code removal: Removing the len(stopTimes) > 0 guard inside the loop (always true) is a nice cleanup.
• Correct caller interaction: Returning "" is properly handled at line 113 (if nextStopID != "") — the NextStop field simply isn't set, which is the right behavior for a completed trip.

Hey Aditya, you've nailed the two items from my first review — the three-case regression test for findNextStop is exactly what was needed, and removing the unused api parameter is a nice cleanup. The core bugfix itself remains correct and well-reasoned.

However, I've found two critical issues that need to be resolved before merging. They're both mechanical fixes — the logic of your change is solid.

Critical

1. CRLF line endings will rewrite both files entirely

Both trips_helper.go and trips_helper_test.go have CRLF (\r\n) line endings, while the rest of the codebase uses LF (\n). This causes GitHub to show 875 additions / 875 deletions for the helper file — the entire file appears rewritten even though only ~10 lines actually changed. More importantly, introducing CRLF into LF files creates inconsistency and guarantees merge conflicts with every other branch touching these files.

Fix: Convert both files back to LF endings. On macOS/Linux:

sed -i 's/\r$//' internal/restapi/trips_helper.go
sed -i 's/\r$//' internal/restapi/trips_helper_test.go

Or configure your editor to use LF for this project. You may also want to add a .gitattributes file or configure your local git: git config core.autocrlf input.

2. Rebase needed — merging will silently remove another contributor's error logging

Because the files are CRLF-rewritten, git treats them as entirely new content. Your branch was created before commit fe3642e ("fix: refine alerts logging and sync doc comments") landed on main, which added important error logging to GetSituationIDsForTrip:

// This code exists on main but NOT on your branch:
} else if !errors.Is(err, sql.ErrNoRows) {
    api.Logger.Warn("Failed to fetch route for alerts; degrading to trip+route matching only",
        slog.String("trip_id", tripID),
        slog.String("route_id", routeID),
        slog.Any("error", err),
    )
}

Merging your PR as-is would quietly overwrite this logging and its associated imports (database/sql, errors, log/slog). That's a regression of another contributor's work.

Fix: After converting to LF endings, rebase on latest main:

# First fix line endings (step 1 above), then:
git fetch origin
git rebase origin/main

The rebase should apply cleanly once the line endings match, since your changes and the logging changes touch different parts of the file. If you run into trouble, here's a guide that may help: https://www.brethorsting.com/blog/2026/01/git-rebase-for-the-terrified/

Verification

After fixing both issues, please confirm:

• make test passes
• make lint reports 0 issues
• go fmt ./... produces no changes
• git diff origin/main --stat shows a small, focused diff (not 875+/875-)

The actual code changes — the bounds-check fix in findNextStop, the regression test, and the api parameter removal — are all correct. Once the line endings and rebase are sorted, this should be good to go.

Hey Aditya, nice work addressing the CRLF and rebase issues from the last round — the diff is now clean and focused, and the alerts logging from the other contributor is properly preserved. Just one more thing to sort out before we can merge:

Critical

1. Tests won't compile — findNextStop signature mismatch (trips_helper_test.go:28)

The test calls findNextStop(stopTimes, vehicle) with 2 arguments, but the function signature still has 3 parameters:

// Current signature (trips_helper.go:294):
func findNextStop(
    api *RestAPI,
    stopTimes []*gtfsdb.StopTime,
    vehicle *gtfs.Vehicle,
) (stopID string, offset int) {

// Test calls (trips_helper_test.go:28, 34, 39):
stopID, _ := findNextStop(stopTimes, vehicle)  // Missing 'api' argument

This means go test ./internal/restapi/... will fail with a compilation error. It looks like the api parameter removal you mentioned in your earlier comment got lost during the CRLF fix and rebase. To fix this, you have two options:

Option A — Remove the unused api parameter (recommended, since it's unused):

// trips_helper.go: Remove 'api *RestAPI' from the signature
func findNextStop(
    stopTimes []*gtfsdb.StopTime,
    vehicle *gtfs.Vehicle,
) (stopID string, offset int) {

// trips_helper.go line 105: Update the call site
nextStopID, nextOffset = findNextStop(stopTimesPtrs, vehicle)

Option B — Add api back to the test calls:

// But this option leaves dead code, so Option A is preferred
stopID, _ := findNextStop(api, stopTimes, vehicle)

After fixing, please confirm go build ./... and make test both pass cleanly.

Thanks again, and I look forward to merging this change.